-
Notifications
You must be signed in to change notification settings - Fork 354
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix memory leaks due to retained tagify, sortable or tooltipster instances #17483
base: master
Are you sure you want to change the base?
Conversation
What should we be looking at in the profiler? I will be honest and admit that for larger performance concerns outside of time spent, these finer features of the debugger are lost on me, and it seems like the heap is actually larger now overall. I'd be fine with the easiest to understand guide you are aware of. Are you aware of the code/known issue/etc for all 3 of these libraries and can you link them? One of them being a leak wouldn't be a surprise, but all 3 is a bit wild. What's the likelihood that these global listeners aren't simply reused for all 3 once registered? |
I am sorry, I should have been more precise in my original description. What you should be looking at in the profiler (I have attached some screenshots of the before/after in the spoiler sections above) is mainly the green line, which represents the number of DOM nodes curently held in memory. Though The orange line (listeners currently in memory) in most cases perfectly correlates to that.
If by that you mean plain JS heap, that one is very volatile and just by running the event loop, some "garbage" will collect, be mainly very shortly lived and then be collected. I have attached screenshot of the memory profile over a 23.5s window. You should see some blue spikes that reset to (mostly) the same low as before. Reason for the periodical increase is the pixi/foundry render loop, which allocates some objects, maybe strings, buffers, etc during the rendering of a frame. The drop part of the spike is when the garbage collector kicks in and all the objects that are not referenced anymore are collected. A more detailed analysis of the profile and testingLets look a the NPC sheet again. I'll create a new profile and concentrate on the important parts and provide some more details and actual values that are not available just looking at the screenshot. This is how I tested it (fresh reload not visible in the video) before: after: The important part here really is not how much memory is currently in use, how many nodes exactly are being allocated on a fresh reload etc. The important part here is that after opening, then closing a sheet, progressing in the encounter tracker etc, the node and listener count drops to exactly the value as before the operation. Everything else means some memory is being retained forever, ever increasing when of of these operations is being performed. (memory leak) Regarding the libraries
There is no issue at all in the Sortable Library. The tagify Libray has some form of broken auto-cleanup which might warrant an issue or PR on that repository. But skimming over the documentation I would still go as far as to say they behave as intended. Tooltipster is slightly more complicated, but we'll get to that. Lets look at the libraries: Sortable
Tagify Tooltipster Finally:
You already noticed, but just to clarify that part or my method a bit more. Yes, that is what the manual GC step (brush or trashcan icon in the performance profiler dev tools tab next to the memory checkbox) is used for. This triggeres a full Garbage collection major cycle (or apparently even two full cycles?). This means that both short lived and long lived objects are iterated over, removing ALL memory that is no longer referenced from the root GC node. I hope I could answer at least some of your questions or concerns. Please do ask further question If something is still unclear :) |
d95d041
to
18abfb2
Compare
18abfb2
to
a3b2420
Compare
3eaf16a
to
9c5cfd4
Compare
9c5cfd4
to
6579942
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks! We're going to wait for a data patch release but will merge soon after.
This PR fixes memory leaks in character/npc/hazard/party/kingdom/item sheets and various other places.
Any application currently using
Sortable
,tooltipster
orTagify
is currently leaking memory (some references + the entire detached application dom tree) because these utilities register global event handlers or other global references that survive the application elements being detached from the DOM.The solution to this to use the respective
destroy
methods these libraries provide. Either.destroy()
on theSortable
orTagify
instances or$(element).tooltipster('destroy')
for tooltipster.This is done by keeping references to the active instances and destroying and nulling them when the application is closed, new event handlers are registered or rule element forms are not used anymore.
The memory profiles in the coming examples were done the following way:
Ideal results are: Three spikes in the allocation that reset to a flat line of exactly the same height as before the spike.
These are the results:
NPC Sheet
before PR
after PR
Character Sheet
before PR
after PR
Hazard Sheet
_Note: Hazard has to be in "unlocked" mode for tagify to be instantianted and triggere the memory leak_before PR
after PR
Kingdom Sheet
_Note: I was unable to test this one as I don't have access to the Kingmaker Premium Module._before PR
N/A
after PR
N/A
Party Sheet
before PR
after PR
IWR Editor
before PR
after PR
Compendium Browser
_Note: This requires the compendiom browser to be opened with a tab that has tagify instances. The Equipment tab for example._before PR
after PR
Pick a Thing Prompt
_Note: TODO describe which thing was picked_before PR
after PR
Encounter Tracker
_Note: This occured whenever the listeners were activated again. For testing, the encounter turn was advanced by one step._before PR
after PR
Note for Rule Elements: This was tested by creating the rule element for a standalone item, then deleting the rule element again without closing the item sheet. Even closing the item sheet however, memory was not freed.
Rule Element Forms: Actor Traits
_Note: TODO - how to test?_before PR
after PR
Rule Element Forms: Aura
before PR
after PR
Rule Element Forms: Fast Healing
_Note: Type has to be set to "regeneration" for memory leak to trigger_before PR
after PR
Rule Element Forms: Roll Note
before PR
after PR
Campaign Feature
before PR
after PR
Deity Sheet
before PR
after PR
Feat Sheet
before PR
after PR
Spell Sheet
before PR
*after PR
Scene Config Sheet
before PR
after PR
Homebrew Language Settings sheet
before PR
after PR
Skill Check Prompt Macro
before PR
after PR